refactor(core): drop unused role parameter from message - #3045
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
如果 AssistantMessage 确认清理该参数,则 Msg 对应的其他几个子类也应同步处理。 |
2cc93cb to
04e3da8
Compare
感谢,已同步处理 |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Removes the unused role parameter from the four @JsonCreator constructors in agentscope-core/message. The cleanup itself is fine — each subtype already pins its role before super(...). But this touches the persistence format of Msg (which is a State) and the PR ships with no test, so I want a round-trip deserialization test before this lands. Also note the CLA check is currently pending for this PR, so it cannot be approved yet regardless.
Automated review by github-manager-bot
| @@ -72,7 +72,6 @@ public AssistantMessage(List<ContentBlock> blocks) { | |||
| private AssistantMessage( | |||
| @JsonProperty("id") String id, | |||
| @JsonProperty("name") String name, | |||
There was a problem hiding this comment.
Dropping @JsonProperty("role") from these @JsonCreators is only safe because Msg is annotated @JsonIgnoreProperties(ignoreUnknown = true) while @JsonTypeInfo(... include = EXISTING_PROPERTY, property = "role", visible = true) still leaves the role field in the stream. Msg persists agent state that is later recovered from the state store, so a Jackson upgrade or an annotation change that stops the inheritance of ignoreUnknown would turn this into a hard deserialization failure on already-persisted payloads. Please add a round-trip regression test that deserializes a JSON document containing "role":"assistant" into each of the four subtypes (and a Msg-typed read) so this cleanup is actually pinned down — right now the PR has no test.
| @@ -77,7 +77,6 @@ public ToolResultMessage(List<ToolResultBlock> results) { | |||
| private ToolResultMessage( | |||
There was a problem hiding this comment.
Two process points: (1) the PR title says AssistantMessage but SystemMessage, UserMessage and ToolResultMessage change too — please retitle (e.g. refactor(core): drop unused role parameter from message @JsonCreators) since core-interface changes cascade to harness/distribution/extensions; (2) the body's "mvn test passed" line sits above the checklist and the checklist is left at template defaults, so the validation evidence is ambiguous. A mvn -pl agentscope-core test` line would be enough.
@seuCHENGHQ 签一下 cla |
04e3da8 to
6e1aef7
Compare
88e18c1 to
9bf14af
Compare
已签署,感谢提醒 |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Removes the unused role parameter from the private @JsonCreator constructors of the four concrete Msg subtypes. Confirmed in the head commit that each constructor already ignored the parameter and passed a hardcoded constant to super (e.g. super(id, name, MsgRole.ASSISTANT, ...)), so this deletes genuinely dead parameters rather than changing which role a built message carries. Left as COMMENT because it touches the persisted message format and has no test covering it.
Findings
- [Warning]
AssistantMessage.java:73—Msgdeclares@JsonTypeInfo(include = As.EXISTING_PROPERTY, property = "role", visible = true), sorolefrom the JSON payload is actively forwarded to the subtype creator. Dropping the parameter makes it an unbindable property that is discarded only because@JsonIgnoreProperties(ignoreUnknown = true)onMsgapplies. Nothing in these four files pins that guarantee, and two behavioural consequences follow that no current test covers:- a stored payload with a missing or mismatched
roleis now silently overridden by the subtype constant instead of surfacing the mismatch; Msg.validateRoleContent(role, content)now always validates against the hardcoded role, so a corrupt persisted message can no longer be rejected on role grounds.
- a stored payload with a missing or mismatched
- [Info]
ToolResultMessage.java:78— same coupling, and this is the subtype where role/content validation is narrowest, so the fixed-vs-supplied role distinction matters most for session recovery.
Suggestions
A single wire-compatibility test would cover all four subtypes and make the invariant explicit instead of relying on an inherited annotation on the base class:
// for each subtype: role correct / role mismatched / role absent
Msg restored = JsonUtils.getJsonCodec().fromJson(payload, Msg.class);
assertEquals(expectedRole, restored.getRole());
assertInstanceOf(ExpectedSubtype.class, restored);Deserializing through Msg.class exercises the @JsonSubTypes resolution path that state recovery actually uses, so it is the more valuable of the two forms.
CI note
The red check is build (windows-latest) and it fails in agentscope-service/service-dataplane — ToolConfirmationCoordinatorTest.replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId:452 CannotStubVoidMethodWithReturnValue. That module is untouched by this diff, so it looks like an unrelated/flaky windows failure, but it does need to be green (or explained by a maintainer) before this can merge.
Automated review by github-manager-bot
| @@ -72,7 +72,6 @@ public AssistantMessage(List<ContentBlock> blocks) { | |||
| private AssistantMessage( | |||
| @JsonProperty("id") String id, | |||
There was a problem hiding this comment.
[Warning] Removing this parameter does change deserialization behaviour, because Msg declares @JsonTypeInfo(..., visible = true) — the role value from JSON is actively forwarded to the subtype creator rather than consumed only as the type discriminator.
With the parameter gone, role is no longer bindable by this creator, so it is dropped as an unknown property. That is tolerated only because Msg carries @JsonIgnoreProperties(ignoreUnknown = true) and that annotation is @Inherited. So the code compiles and round-trips today, but the cleanup now depends on an inherited annotation on the base class that nothing in this file enforces: if ignoreUnknown were ever narrowed, deserializing these four subtypes would start failing with UnrecognizedPropertyException.
Two consequences worth locking down with a wire-compat test before merge:
- A payload whose
roleis absent or whose value does not match the resolved subtype is now silently overridden by the hardcoded constant (e.g. anASSISTANTpayload always yieldsMsgRole.ASSISTANT). That is the intent, but it is an implicit guarantee. Msg.validateRoleContent(role, content)now always sees the hardcoded role, so a mismatched stored payload can no longer be detected at all.
A test that deserializes each subtype from JSON with a correct role, a mismatched role, and a missing role, asserting the resulting getRole() and that no exception is thrown, would make the invariant explicit and independent of the base-class annotation.
| @@ -77,7 +77,6 @@ public ToolResultMessage(List<ToolResultBlock> results) { | |||
| private ToolResultMessage( | |||
| @JsonProperty("id") String id, | |||
There was a problem hiding this comment.
[Info] Same visible = true coupling as AssistantMessage.java:73. This subtype is the one where it matters most: ToolResultMessage content blocks are the narrowest (validateRoleContent restricts TOOL), so the role that reaches validation is now fixed rather than supplied by the stored payload — strictly an improvement for state recovery from AgentStateStore, but again only implicitly.
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Removes the unused role parameter from the private @JsonCreator constructors of the four concrete Msg subtypes. Confirmed in the head commit that each constructor already ignored the parameter and passed a hardcoded constant to super (e.g. super(id, name, MsgRole.ASSISTANT, ...)), so this deletes genuinely dead parameters rather than changing which role a built message carries. Left as COMMENT because it touches the persisted message format and has no test covering it.
Findings
- [Warning]
AssistantMessage.java:73—Msgdeclares@JsonTypeInfo(include = As.EXISTING_PROPERTY, property = "role", visible = true), sorolefrom the JSON payload is actively forwarded to the subtype creator. Dropping the parameter makes it an unbindable property that is discarded only because@JsonIgnoreProperties(ignoreUnknown = true)onMsgapplies. Nothing in these four files pins that guarantee, and two behavioural consequences follow that no current test covers:- a stored payload with a missing or mismatched
roleis now silently overridden by the subtype constant instead of surfacing the mismatch; Msg.validateRoleContent(role, content)now always validates against the hardcoded role, so a corrupt persisted message can no longer be rejected on role grounds.
- a stored payload with a missing or mismatched
- [Info]
ToolResultMessage.java:78— same coupling, and this is the subtype where role/content validation is narrowest, so the fixed-vs-supplied role distinction matters most for session recovery.
Suggestions
A single wire-compatibility test would cover all four subtypes and make the invariant explicit instead of relying on an inherited annotation on the base class:
// for each subtype: role correct / role mismatched / role absent
Msg restored = JsonUtils.getJsonCodec().fromJson(payload, Msg.class);
assertEquals(expectedRole, restored.getRole());
assertInstanceOf(ExpectedSubtype.class, restored);Deserializing through Msg.class exercises the @JsonSubTypes resolution path that state recovery actually uses, so it is the more valuable of the two forms.
CI note
The red check is build (windows-latest) and it fails in agentscope-service/service-dataplane — ToolConfirmationCoordinatorTest.replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId:452 CannotStubVoidMethodWithReturnValue. That module is untouched by this diff, so it looks like an unrelated/flaky windows failure, but it does need to be green (or explained by a maintainer) before this can merge.
Automated review by github-manager-bot
| @@ -72,7 +72,6 @@ public AssistantMessage(List<ContentBlock> blocks) { | |||
| private AssistantMessage( | |||
| @JsonProperty("id") String id, | |||
There was a problem hiding this comment.
[Warning] Removing this parameter does change deserialization behaviour, because Msg declares @JsonTypeInfo(..., visible = true) — the role value from JSON is actively forwarded to the subtype creator rather than consumed only as the type discriminator.
With the parameter gone, role is no longer bindable by this creator, so it is dropped as an unknown property. That is tolerated only because Msg carries @JsonIgnoreProperties(ignoreUnknown = true) and that annotation is @Inherited. So the code compiles and round-trips today, but the cleanup now depends on an inherited annotation on the base class that nothing in this file enforces: if ignoreUnknown were ever narrowed, deserializing these four subtypes would start failing with UnrecognizedPropertyException.
Two consequences worth locking down with a wire-compat test before merge:
- A payload whose
roleis absent or whose value does not match the resolved subtype is now silently overridden by the hardcoded constant (e.g. anASSISTANTpayload always yieldsMsgRole.ASSISTANT). That is the intent, but it is an implicit guarantee. Msg.validateRoleContent(role, content)now always sees the hardcoded role, so a mismatched stored payload can no longer be detected at all.
A test that deserializes each subtype from JSON with a correct role, a mismatched role, and a missing role, asserting the resulting getRole() and that no exception is thrown, would make the invariant explicit and independent of the base-class annotation.
| @@ -77,7 +77,6 @@ public ToolResultMessage(List<ToolResultBlock> results) { | |||
| private ToolResultMessage( | |||
| @JsonProperty("id") String id, | |||
There was a problem hiding this comment.
[Info] Same visible = true coupling as AssistantMessage.java:73. This subtype is the one where it matters most: ToolResultMessage content blocks are the narrowest (validateRoleContent restricts TOOL), so the role that reaches validation is now fixed rather than supplied by the stored payload — strictly an improvement for state recovery from AgentStateStore, but again only implicitly.
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
The "role" parameter is not being used within the method of AssistantMessage.
Checklist
mvn test passed
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)